fix(parser): bind source-counter-gated rider pronouns to the source (Gemstone Mine #6507) - #6559
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe parser now detects source-scoped counters through nested quantity expressions, rewrites leading bare-recipient counter conditions after typed targets, and adjusts pronoun and parent-target binding. Parser and integration tests cover related counter and sacrifice scenarios. ChangesOracle counter binding
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/tests.rs (1)
28634-28709: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrapper coverage is partial: only
Ref/Offsetare exercised.
quantity_expr_reads_source_countershas 8 recursive wrapper arms (DivideRounded,Offset,ClampMin,Multiply,Sum,Max,UpTo,Power,Difference), but this test only drives propagation throughOffset. Consider adding a couple more positive cases (e.g.Sum/Difference, which take two sub-expressions and are the likeliest place for a copy-paste slip) to lock in the exhaustive-walk guarantee the doc comment advertises.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/tests.rs` around lines 28634 - 28709, Add positive test cases in condition_refs_source_object_source_counter_quantity_check covering additional quantity_expr_reads_source_counters wrappers, especially binary Sum and Difference expressions containing a source-scoped CountersOn reference. Keep the assertions focused on propagation through both operands and preserve the existing non-source and wrapper coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 28634-28709: Add positive test cases in
condition_refs_source_object_source_counter_quantity_check covering additional
quantity_expr_reads_source_counters wrappers, especially binary Sum and
Difference expressions containing a source-scoped CountersOn reference. Keep the
assertions focused on propagation through both operands and preserve the
existing non-source and wrapper coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4862da73-1499-4d5a-b7f0-3eb506842b5d
📒 Files selected for processing (5)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_tests.rscrates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rscrates/engine/tests/integration/main.rs
Parse changes introduced by this PR · 18 card(s), 6 signature(s) (baseline: main
|
|
Request changes — the diagnosis, the seam, and the CR work are all correct, but the predicate is one degree too broad and lands a functional regression on Revelation of Power, a card outside the claimed scope. 🔴 Blocker
|
matthewevans
left a comment
There was a problem hiding this comment.
Current head 11b490a remains blocked.
condition_refs_source_object treats every QuantityCheck containing CountersOn { scope: Source } as a source reference. That is over-broad: Revelation of Power's bare "it" refers to its target creature, so this predicate rebinds the conditional flying/lifelink rider to the Instant itself and drops previously working behavior.
Narrow this to an explicit source noun phrase or suppress the path when the chain already carries the chosen-target referent. Add the Revelation of Power regression test and regenerate/reconcile the parse-diff before requesting review again.
… chosen target (phase-rs#6559 review) The phase-rs#6507 predicate that binds a source-counter-gated rider pronoun to SelfRef was one degree too broad: it also fired on Revelation of Power ("Target creature gets +2/+2 until end of turn. If it has a counter on it, it also gains flying and lifelink"), whose intervening-if mis-scopes the bare "it" to CountersOn{Source}. Binding that grant to the source dropped flying/lifelink onto the one-shot Instant — the card lost its second sentence (engine_regress), and CR 608.2k does not reach it (the source is named by neither a cost nor a trigger condition). Narrow the binding: only rebind the pronoun to the source when NO earlier clause in the chain chose a typed target. Compute one gate at the chunk-subject site and reuse it at both consumers (the chunk-subject binding and the replace_target_with_parent guard): let binds_source_counter_pronoun = condition .is_some_and(condition_refs_source_object) && !chain_has_prior_typed_referent(builder.clauses(), false); chain_has_prior_typed_referent is true for Revelation of Power (its prior "Target creature gets +2/+2" is a Pump over a typed target) and false for every depletion-land / counter rider (whose prior clause is "Add mana" or "put a counter on ~", never a chosen target), so all 21 intended heals keep SelfRef while Revelation of Power's grant returns to ParentTarget. Chosen deliberately over chain_prior_referent_is_chosen_target, whose has_typed_target_widened early-out returns false for a pump-of-a-target. Adds source_counter_gate_over_prior_target_keeps_parent_not_self_ref (pins Revelation of Power's grant to ParentTarget) and extends the predicate unit test to drive the Sum/Difference two-operand walker arms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thank you — the Revelation of Power catch was exactly right, and the diagnosis ("a mis-scoped bare Blocker — Revelation of Power no longer regressesI took the suppress-when-a-prior-clause-chose-a-typed-target option. At the let binds_source_counter_pronoun = condition
.as_ref()
.is_some_and(condition_refs_source_object)
&& !chain_has_prior_typed_referent(builder.clauses(), false);
I chose
Added Non-blocking items
Full lib + integration suites green; parser-combinator gate (Gate A) passes; the branch is up to date with current |
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current-head production coverage gap.
🟠 Required
The repair for the prior Revelation of Power regression still lacks a production-pipeline regression. crates/engine/src/parser/oracle_effect/mod.rs:29929-29948 changes the runtime target binding and :30725-30743 rewrites the parent target, but Revelation is covered only by parser-shape tests in oracle_tests.rs:22924-22981; the new integration cases exercise Gemstone Mine, Peat Bog, and Last Light. The original defect was visible only after casting, target propagation, and layer application, where the grant could be applied to the Instant rather than the selected creature. Add an integration test that casts Revelation of Power at a countered creature and asserts that creature receives flying and lifelink; make it discriminating against reversion of the new guard.
🟡 Also reconcile before re-review
The sole parse-diff artifact predates this head and still describes Revelation as changing to self; the fresh Card data job is in progress. Let it publish and reconcile the exact affected-card/signature set for this head.
Recommendation: add the end-to-end Revelation regression, then provide current-head parse-diff evidence before re-review.
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/engine/src/game/engine.rs (1)
9710-9723: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
NeedsChoice(player)shadows the land-playing player passed to the finalizer.The
ReplacementResult::NeedsChoice(player)pattern at line 9710 binds the player who must answer the replacement choice. That binding shadows the outerplayerresolved at lines 9317-9326, which is the land-playing player (acting_playerunder shared team turns, otherwiseturn_control::turn_resource_owner).Line 9716 passes the shadowed value to
finalize_committed_land_play. The finalizer then attributes the land drop to the chooser: it increments that player'slands_played_this_turn(line 9259) and emitsLandPlayed { player_id: player }(lines 9261-9265). When the chooser is not the land-player, the once-per-turn land allowance is charged to the wrong player and the event feed misattributes the play.The arm needs both identities: line 9725 correctly passes the chooser to
replacement_choice_waiting_for. Rename the pattern binding so the two do not collide.🐛 Proposed fix separating the two identities
- super::replacement::ReplacementResult::NeedsChoice(player) => { + super::replacement::ReplacementResult::NeedsChoice(choosing_player) => { // A replacement needs player choice (e.g., shock land "pay 2 life?"). // Increment counters now — the land play is committed, only the ETB // effect is pending. finalize_committed_land_play( state, player, object_id, origin_zone, gy_permission_source, exile_play_authorization, library_permission_src, events, ); return Ok(super::replacement::replacement_choice_waiting_for( - player, state, + choosing_player, state, )); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/engine.rs` around lines 9710 - 9723, Rename the player binding in the ReplacementResult::NeedsChoice arm so it does not shadow the outer land-playing player resolved by the surrounding function. Continue passing the outer player to finalize_committed_land_play, while passing the renamed choice-player binding to replacement_choice_waiting_for.client/src/hooks/__tests__/useConcedeHandler.test.tsx (1)
168-191: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that the unbound path calls neither
sendConcedenordispatch.The fixture installs
{ sendConcede: vi.fn() }to model an adapter without the match capability. The test never asserts that thissendConcedestayed uncalled, and it never assertsdispatchMockstayed uncalled.Those two assertions are the guarantee the change adds: the draft-pod branch must not fall through to a game-level concession. Hold a reference to the mock and assert on it.
🧪 Proposed change
- adapterForTest = { sendConcede: vi.fn() }; + const fallbackSendConcede = vi.fn(); + adapterForTest = { sendConcede: fallbackSendConcede };expect(clearGameMock).not.toHaveBeenCalled(); expect(navigateMock).not.toHaveBeenCalled(); + expect(fallbackSendConcede).not.toHaveBeenCalled(); + expect(dispatchMock).not.toHaveBeenCalled();As per path instructions: "A test must exercise the FAILURE path the fix prevents".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/hooks/__tests__/useConcedeHandler.test.tsx` around lines 168 - 191, Update the unbound draft-pod test around useConcedeHandler to retain the sendConcede mock reference and assert it is not called after invoking result.current(). Also assert dispatchMock is not called, preserving the existing clearGameMock and navigateMock assertions to verify the branch does not fall through to game-level concession.Source: Path instructions
client/src/adapter/p2p-adapter.ts (1)
351-369: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOrphaned
pendingViewsentries stall native revision fan-out.A revision entry is only deleted when
views.size === this.clients.size.detachGuestremoves a client fromthis.clientsbut leaves that client's partially filled entries inpendingViews. Two consequences follow.
- A revision that was in flight when a seat detached never reaches the size check again, so
onRevisionnever runs for it. Guests do not receive that state frame.- The entry stays in
pendingViewsfor the lifetime of the bridge.The size comparison is also fragile in the other direction:
attachGuestincreasesclients.sizewhile earlier revisions are still partial.Prune stale revisions on detach and re-evaluate completeness against the current client set.
🔧 Proposed fix in `detachGuest`
detachGuest(playerId: PlayerId): void { if (playerId === 0) return; this.clients.get(playerId)?.dispose(); this.clients.delete(playerId); this.playerTokens.delete(playerId); this.latestViews.delete(playerId); + // A revision that was still collecting this seat's view can never reach + // `clients.size` again. Flush the ones that are now complete and drop the + // seat from the rest. + for (const [revision, views] of this.pendingViews) { + views.delete(playerId); + if (views.size === this.clients.size) { + this.pendingViews.delete(revision); + this.revisionQueue = this.revisionQueue + .then(() => this.onRevision(revision, views)) + .catch((error) => { + console.error("[NativeP2PBridge] revision fan-out failed:", error); + }); + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/adapter/p2p-adapter.ts` around lines 351 - 369, Update detachGuest to remove the detached player’s entries from every pendingViews revision, delete revisions that become empty, and re-evaluate remaining revisions against the current clients set so complete revisions invoke onRevision. Ensure attachGuest or the revision-processing path also rechecks partial revisions against the current client set, preventing client-count changes from leaving stale or prematurely blocked fan-out entries.crates/engine/src/game/effects/mod.rs (1)
5860-5889: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the
n == 0guard intodrive_sequential_repeated_optional_paymentor usesaturating_sub.
drive_sequential_repeated_optional_paymentcomputesremaining: (n - 1) as u32. The only thing that keepsn >= 1is theif n == 0 { return Ok(()) }check that stayed behind indrive_repeated_optional_payment. The new function is now a separate entry point with the arithmetic and the guard in different places. If it is called withn == 0,(0 - 1) as u32isu32::MAX, and the repeated-payment frame offers effectively unboundedOptionalEffectChoiceprompts.Make the function safe on its own inputs.
🐛 Proposed fix
fn drive_sequential_repeated_optional_payment( state: &mut GameState, ability: &ResolvedAbility, reflexive: &ResolvedAbility, n: i32, ) -> Result<(), EffectError> { + // CR 603.12a: the payment budget is at least one offer; a zero budget never + // opens the process (the caller returns early). + let Ok(budget) = u32::try_from(n) else { + return Ok(()); + }; + let Some(remaining) = budget.checked_sub(1) else { + return Ok(()); + };- remaining: (n - 1) as u32, + remaining,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/effects/mod.rs` around lines 5860 - 5889, Make drive_sequential_repeated_optional_payment safe when n is zero by returning Ok(()) before constructing the payment frame, or by using saturating subtraction for remaining. Keep the existing behavior for positive n and ensure no zero-count call can create a frame with an effectively unbounded remaining value.
🟡 Minor comments (11)
crates/phase-ai/src/bin/ai_commander.rs-1050-1070 (1)
1050-1070: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the doc comment: measurement mode is no longer unconditional.
The first paragraph states that EVERY seat runs in measurement mode.
build_seat_confignow selects the mode fromrun_context, andRunContext::Interactivekeeps the wall-clock deadline. Scope the paragraph to the measurement route so the summary matches the code.📝 Proposed doc correction
-/// EVERY seat runs in MEASUREMENT mode (`AiConfig::into_measurement`), which -/// disables the wall-clock search deadline (`AI_SEARCH_TIME_BUDGET_MS`, default +/// Under `RunContext::Measurement`, every seat runs with +/// `AiConfig::into_measurement`, which +/// disables the wall-clock search deadline (`AI_SEARCH_TIME_BUDGET_MS`, default /// 1500ms) so search is bounded SOLELY by `max_nodes`/`max_depth`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-ai/src/bin/ai_commander.rs` around lines 1050 - 1070, Update the documentation in build_seat_config to scope the measurement-mode behavior to seats configured through the measurement route, rather than claiming every seat uses AiConfig::into_measurement. Explicitly preserve that RunContext::Interactive retains the wall-clock search deadline, while measurement mode remains bounded by max_nodes/max_depth for reproducibility.crates/engine/src/game/mana_sources.rs-2388-2394 (1)
2388-2394: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winColorless intrinsic land production stays ungated.
mana_type_to_colorreturnsNonefor a colorless mana type, sois_some_andyieldsfalseandblockedstaysfalse. A land whose only mana source is a granted colorless basic land subtype (Wastes class) therefore bypassesintrinsic_land_mana_ability_blockedentirely, which is the same gate-bypass class this change closes for colored subtypes (issue#6469).Route the colorless case through the same readiness check, or record in the comment that
intrinsic_land_mana_ability_definitionaccepts only aManaColorand that colorless intrinsic production is intentionally out of scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/mana_sources.rs` around lines 2388 - 2394, Update the blocked calculation near intrinsic land mana handling so colorless mana types also go through the appropriate readiness gate instead of being skipped when mana_type_to_color returns None. Reuse intrinsic_land_mana_ability_blocked where possible, or explicitly document in the surrounding logic that intrinsic_land_mana_ability_definition only supports ManaColor and colorless production is intentionally excluded.crates/server-core/src/game_action_payload_guard.rs-568-570 (1)
568-570: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRejection messages will name the wrong action for
ActivateManaSource.
guard_mana_source_selection_payloadhardcodes the"TapLandForMana.selection…"prefix in every error string (lines 207, 212, 217, 222). A hostileActivateManaSourcepayload is now rejected with a field path that names an action the client did not send. Pass the action label into the helper so the reason matches the rejected variant.🔧 Proposed fix
- GameAction::TapLandForMana { selection } | GameAction::ActivateManaSource { selection } => { - guard_mana_source_selection_payload(selection)?; - } + GameAction::TapLandForMana { selection } => { + guard_mana_source_selection_payload("TapLandForMana", selection)?; + } + GameAction::ActivateManaSource { selection } => { + guard_mana_source_selection_payload("ActivateManaSource", selection)?; + }Then build each label from the passed action name inside the helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/server-core/src/game_action_payload_guard.rs` around lines 568 - 570, Update guard_mana_source_selection_payload and its call site in the GameAction match to accept the action label, passing the appropriate label for TapLandForMana or ActivateManaSource. Build every rejection field path inside the helper from that label instead of hardcoding “TapLandForMana.selection”, so each error identifies the actual rejected action.client/src/components/multiplayer/ConcedeDialog.tsx-58-75 (1)
58-75: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRaise the button height to meet the 44pt touch target.
px-5 py-2 text-smproduces roughly a 36px tall control. The path instructions require touch targets of at least 44pt. Both changed buttons use this padding.Add
min-h-11(44px) and center the label.🛠️ Proposed change
<button onClick={gameAction.onConfirm} - className="rounded-lg bg-red-600 px-5 py-2 text-sm font-semibold text-white transition hover:bg-red-500" + className="min-h-11 rounded-lg bg-red-600 px-5 py-2 text-sm font-semibold text-white transition hover:bg-red-500" >Apply the same
min-h-11to the match button at line 71.As per path instructions: "Touch targets >= 44pt".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/components/multiplayer/ConcedeDialog.tsx` around lines 58 - 75, Update both the game and match confirmation buttons in ConcedeDialog to include min-h-11 and vertically center their labels, preserving their existing styling and behavior.Source: Path instructions
client/src/adapter/__tests__/p2pDraftHostBo3.test.ts-437-445 (1)
437-445: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis test exercises the
!pairingbranch, not the round-currency branch.
binding.roundis 2 (line 340).setHostViewsuppliescurrent_round: 2, soview.current_round !== binding.roundis false. The pairing lookup inacceptMatchSettlementmatches onmatch_idandcandidate.round === binding.round, and the fixture suppliespairing("m-12", 1, 1, 2)with round 1. The lookup therefore returnsundefinedand the rejection comes from!pairing.The
view.current_round !== binding.roundguard stays untested. Add a case that keeps the pairing atbinding.roundand advancescurrent_round.🧪 Proposed added case
+ it("rejects a bound settlement once the pod advanced past its round", async () => { + setHostView({ + current_round: 3, + pairings: [pairing("m-12", 2, 1, 2)], + }); + await deliverSettlement(1); + expect(reportSpy).not.toHaveBeenCalled(); + expect(sent.get(1)).toEqual([ + { type: "draft_error", reason: "Unauthorized match settlement" }, + ]); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/adapter/__tests__/p2pDraftHostBo3.test.ts` around lines 437 - 445, Update the test around the settlement rejection case so the pairing returned by pairing("m-12", ...) uses binding.round (2), while setHostView.current_round is advanced to a different round (for example, 3). Keep the settlement delivery and rejection assertions, ensuring acceptMatchSettlement exercises the view.current_round !== binding.round guard rather than the !pairing branch.client/src/network/protocol.ts-95-110 (1)
95-110: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReconcile the duplicated version-17 log entries.
The version log now has two separate
17headings: line 95 for sacrificial-mana source selection, and line 103 for bound draft-match concession. The second one is also placed between the7and6entries, which breaks the descending order and hides it from a reader scanning the top of the list.WIRE_PROTOCOL_VERSIONis a single integer, so both changes ship as one version and must be described in one entry.This version also adds the authority stamp,
sessionKeyonreconnect,revisionongame_setup/state_update/reconnect_ack, and theterminal_resultframe. None of those appear in the log. A future author bumping to 18 can miss one of the two17blocks.📝 Proposed consolidation
- * 17 — Sacrificial-mana source selection action and waiting-state snapshots. + * 17 — Sacrificial-mana source selection action and waiting-state snapshots; + * host authority stamp on every host-originated frame; reconnect + * sessionKey; state revisions on game_setup/state_update/reconnect_ack; + * recipient-scoped terminal_result frames; bound draft-match concession + * request (a Traditional-draft guest asks its match authority to settle + * the match instead of sending a game-level concession). * 12 — Connive exact subject snapshots and resident paused post-replacement * drains changed P2P GameState snapshots. * 11 — Serialized GameState trigger provenance and paused logical zone-change owners. * 10 — Dedicated companion deck slot and typed companion-reveal choices. * 9 — Meld pair and attacking-entry choices after mana-payment preview variants. * 8 — Mana-payment preview request/response variants. * 7 — PrecastCopyShortcut action and its two WaitingFor variants. - * 17 — Bound draft-match concession request. A Traditional-draft guest - * asks its match authority to settle the match; it must not send a - * game-level concession through the ordinary P2P path. * 6 — Mulligan bottoming folded into a MulliganDecisionPhase::BottomCards🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/network/protocol.ts` around lines 95 - 110, Consolidate the duplicate version-17 entries in the protocol version history comment into one top-level entry, preserving descending order and covering all version-17 changes: sacrificial-mana source selection, bound draft-match concession, authority stamping, reconnect sessionKey, revision fields, and the terminal_result frame. Keep WIRE_PROTOCOL_VERSION unchanged at 17.crates/engine/src/game/match_flow.rs-295-313 (1)
295-313: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA forfeit by a player who already holds two game wins produces a 2-2 frozen score.
The guards accept any state whose
match_phaseis notCompleted. Ifp0_winsis already 2 while the phase is stillInGame— the window beforehandle_game_over_transitionruns for the clinching game — andPlayerId(0)forfeits, Line 301 setsp1_wins = 2. The recordedmatch_scorebecomes 2-2 whilematch_forfeit_result.winnerisPlayerId(1), and that inconsistent score is what terminal presentation freezes and shows.Reject the forfeit when the opponent has already clinched the match, so the earned result is never overwritten.
🐛 Proposed fix
let winner = match forfeiting_player { PlayerId(0) => PlayerId(1), PlayerId(1) => PlayerId(0), _ => return Err("Forfeiting player is not a match seat".to_string()), }; + // A seat that has already won the match cannot hand it to its opponent: + // clamping the opponent to two wins would record a 2-2 score. + let forfeiting_wins = match forfeiting_player { + PlayerId(0) => state.match_score.p0_wins, + _ => state.match_score.p1_wins, + }; + if forfeiting_wins >= 2 { + return Err("Forfeiting player has already won the match".to_string()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/match_flow.rs` around lines 295 - 313, Update the forfeit handling before the score mutation in the match transition flow to reject a forfeit when the forfeiting player’s opponent already has the clinching game count (two wins), even if match_phase is not yet Completed. Preserve the existing score and winner state in that case, and only execute the match_forfeit_result and frozen-score updates for valid forfeits.crates/phase-server/src/persistence.rs-403-423 (1)
403-423: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe activation upsert accepts an equal generation and can overwrite a live snapshot with revision 0.
The guard at Line 414 is
excluded.generation >= game_sessions.generation. The earlier check at Line 391 only rejects a strictly greater stored generation. An activation for the samegame_codeand the samegenerationtherefore passes both checks and replacessession_jsonandmutation_revisionwith the freshly created values, discarding the retained state of the row already at that generation.
create_full_session_keyrefuses to allocate while a non-retired row exists, so the path is not reachable today. Rated by effect when reached, this is snapshot loss for an active session. Make the predicate strict, and let the caller treat a non-Applieddisposition as the conflict it is.🐛 Proposed fix
- WHERE excluded.generation >= game_sessions.generation", + WHERE excluded.generation > game_sessions.generation + OR game_sessions.retired = 1",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-server/src/persistence.rs` around lines 403 - 423, Update the conflict predicate in the game-session activation upsert to require excluded.generation to be strictly greater than game_sessions.generation, preventing equal-generation snapshots from overwriting retained state. In the surrounding activation caller, handle any disposition other than Applied as a conflict, preserving existing behavior for successfully applied activations.crates/engine/src/parser/oracle_tests.rs-23286-23320 (1)
23286-23320: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the reach-guards this test's three siblings carry.
explicit_source_counter_gate_over_prior_target_stays_source_scopedasserts a scope value without proving the rider reached the code under test. Its siblingsdepletion_land_rider_sacrifice_binds_self_ref,typed_trigger_source_counter_rider_binds_self_ref_not_triggering_source, andsource_counter_gate_over_prior_target_keeps_parent_not_self_refall assert zero parse warnings and zeroEffect::Unimplementedfirst.Without those guards, a rider that misparsed into an
Unimplementedeffect while still carrying aCountersOn { scope: Source }condition satisfies thematches!and the test passes for the wrong reason. This test is the discriminating half of the pair with the Revelation of Power guard, so a false green here removes the only evidence that the suppression keys on the bare anaphor rather than on "a prior typed target exists".💚 Proposed reach-guards and a positive rider-shape assertion
); + assert!( + parsed.parse_warnings.is_empty(), + "expected zero parse warnings, got {:#?}", + parsed.parse_warnings + ); + fn has_unimpl(def: &AbilityDefinition) -> bool { + matches!(def.effect.as_ref(), Effect::Unimplemented { .. }) + || def.sub_ability.as_deref().is_some_and(has_unimpl) + } + assert!( + !parsed.abilities.iter().any(has_unimpl), + "no Unimplemented anywhere in the parse: {:#?}", + parsed.abilities + ); let rider = parsed.abilities[0] .sub_ability .as_deref() .expect("conditional flying rider"); + assert!( + matches!(rider.effect.as_ref(), Effect::GenericEffect { .. }), + "the rider must remain the flying grant, got {:?}", + rider.effect + ); assert!(As per path instructions: "For every negative assertion … require a paired positive reach-guard proving the input actually reached the code under test (parse succeeded, zero
Effect::Unimplemented, expected positive shape); an upstream short-circuit makes a negative pass for the wrong reason."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_tests.rs` around lines 23286 - 23320, Update explicit_source_counter_gate_over_prior_target_stays_source_scoped to add reach guards before the scope match: assert zero parse warnings and zero Effect::Unimplemented effects, then assert the rider’s expected positive shape before checking CountersOn with ObjectScope::Source. Mirror the guard pattern used by depletion_land_rider_sacrifice_binds_self_ref, typed_trigger_source_counter_rider_binds_self_ref_not_triggering_source, and source_counter_gate_over_prior_target_keeps_parent_not_self_ref.Source: Path instructions
crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs-982-993 (1)
982-993: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAccepting
Effect::Unimplementedmakes this assertion non-discriminating.The match arm at Line 987 passes when the leave-battlefield rider is completely unmodeled. The test then reports success for both the fixed shape (
AddTargetReplacement{TrackedSet}) and the unfixed shape. Assert the intended shape only, or split the tolerance into a separate#[ignore]d or explicitly documented expected-gap test so a regression toUnimplementedfails.As per path instructions: "Flag constructor shortcuts … that can silently mask the very bug a regression test claims to catch."
🧪 Proposed tightening
match find_leave_rider(execute) { Some(Effect::AddTargetReplacement { target: TargetFilter::TrackedSet { .. }, .. }) => {} - Some(Effect::Unimplemented { .. }) => {} Some(other) => panic!( - "leave-battlefield rider must be AddTargetReplacement{{TrackedSet}} or \ - Unimplemented, got {other:?}" + "leave-battlefield rider must be AddTargetReplacement{{TrackedSet}}, got {other:?}" ), None => panic!("expected leave-battlefield rider in Storm Herald chain"), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs` around lines 982 - 993, Update the leave-battlefield rider assertion around find_leave_rider so only Effect::AddTargetReplacement with TargetFilter::TrackedSet is accepted. Remove the successful Effect::Unimplemented match arm, ensuring an unmodeled rider reaches the existing failure path and the regression test cannot pass for the unfixed behavior.Source: Path instructions
crates/engine/src/game/casting_tests.rs-31223-31317 (1)
31223-31317: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPair the three sibling-gate negatives with a positive reach-guard that uses
add_bare_subtype_forest.
bare_subtype_land_detained_excluded_from_legal_mana_actions,bare_subtype_land_phased_out_excluded_from_legal_mana_actions, andbare_subtype_land_cant_tap_excluded_from_legal_mana_actionsassert only absence fromactivatable_mana_actions_for_player. All three build their land throughadd_bare_subtype_forest. The positive companionbare_subtype_land_still_offers_mana_without_a_prohibitionbuilds its land inline instead of calling that helper. If the helper ever stops producing a valid intrinsic mana source (for example a changed subtype string or a missingentered_battlefield_turn), all three negatives pass for the wrong reason and the positive test still passes.Add a positive assertion inside each gate test before applying the gate, or route the positive companion through the same helper.
💚 Proposed reach-guard inside one gate test
let mut state = setup_game_at_main_phase(); let forest = add_bare_subtype_forest(&mut state, PlayerId(1), 0xF0128); + assert!( + crate::game::mana_sources::activatable_mana_actions_for_player(&state, PlayerId(1)) + .iter() + .any(|action| action.source_object() == Some(forest)), + "reach-guard: the helper-built land must offer its intrinsic mana ability before the gate" + ); state .objects .get_mut(&forest)As per path instructions: "For every negative assertion (
!detector(...), "not applied", "does not parse to X"), require a paired positive reach-guard proving the input actually reached the code under test".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/casting_tests.rs` around lines 31223 - 31317, Ensure the three negative tests using add_bare_subtype_forest first verify that the unmodified forest appears in activatable_mana_actions_for_player, before applying detention, phasing, or CantTap. Alternatively, update bare_subtype_land_still_offers_mana_without_a_prohibition to construct the land through add_bare_subtype_forest, so the shared helper is positively proven to produce an intrinsic mana source before each prohibition assertion.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 680aa6e7-c6ab-4b66-8a2c-48e2dfdb2da6
⛔ Files ignored due to path filters (66)
Cargo.lockis excluded by!**/*.lockclient/public/changelog-meta.jsonis excluded by!client/public/changelog*.jsonclient/public/changelog.jsonis excluded by!client/public/changelog*.jsonclient/src-tauri/Cargo.lockis excluded by!**/*.lockclient/src/adapter/generated/interaction/index.tsis excluded by!**/generated/**client/src/wasm/engine_wasm.d.tsis excluded by!client/src/wasm/**,!**/*.d.tscrates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__aangs_journey_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__abraxas_named_equip_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__abraxas_named_equip_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__aerial_formation_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__aetherling_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__analyze_the_pollen_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__arni_brokenbrow_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__barbarian_ring_activated_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__batterskull_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__birds_of_paradise_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__blunt_the_assault_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__bomat_courier_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__bone_splinters_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__boseiju_who_endures_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__browbeat_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__carbonize_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__case_of_the_stashed_skeleton_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__champions_victory_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__chandra_nalaar_minus_x_loyalty_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__chandra_nalaar_minus_x_loyalty_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__component_pouch_activated_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__conformer_shuriken_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__deadly_rollick_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__dismember_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__evils_thrall_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__experiment_one_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__figure_of_destiny_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__fog_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__follow_the_lumarets_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__ghost_lit_stalker_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__govern_the_guildless_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__guul_draz_assassin_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__incinerate_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__jace_the_mind_sculptor_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__jade_mage_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__joraga_treespeaker_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__liliana_of_the_veil_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__liliana_the_repentant_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__llanowar_elves_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__manamorphose_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__mother_of_runes_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__questing_beast_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__repeat_offender_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__short_sword_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__stoneforge_mystic_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__swords_to_plowshares_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__sylvan_safekeeper_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@full_throttle_temporal_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@full_throttle_temporal_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@galvanic_iteration_temporal_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@galvanic_iteration_temporal_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@pact_of_negation_temporal_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@pact_of_negation_temporal_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__thespians_stage_generic_activated_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__touch_of_the_void_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__village_rites_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__vines_of_vastwood_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__walking_ballista_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/tests/fixtures/cr733/authority_matrix.json.gzis excluded by!**/*.gzlobby-worker/broker-wasm/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (239)
.github/workflows/ai-gate.ymlCargo.tomlTiltfileclient/package.jsonclient/public/feeds/mtggoldfish-commander.jsonclient/public/feeds/mtggoldfish-modern.jsonclient/public/feeds/mtggoldfish-pioneer.jsonclient/public/feeds/mtggoldfish-standard.jsonclient/src-tauri/Cargo.tomlclient/src-tauri/tauri.conf.jsonclient/src/adapter/__tests__/ai-card-subset.test.tsclient/src/adapter/__tests__/ai-worker-pool.test.tsclient/src/adapter/__tests__/manaSourceSelectionWireTypes.test.tsclient/src/adapter/__tests__/p2p-adapter-multiplayer.test.tsclient/src/adapter/__tests__/p2pDraftHostBo3.test.tsclient/src/adapter/__tests__/wasm-adapter.test.tsclient/src/adapter/__tests__/ws-adapter.test.tsclient/src/adapter/ai-worker-pool.tsclient/src/adapter/card-db-subset.tsclient/src/adapter/draftPodGuestAdapter.tsclient/src/adapter/draftPodHostAdapter.tsclient/src/adapter/engine-worker-client.tsclient/src/adapter/engine-worker.tsclient/src/adapter/p2p-adapter.tsclient/src/adapter/p2p-draft-guest.tsclient/src/adapter/p2p-draft-host.tsclient/src/adapter/replay-adapter.tsclient/src/adapter/server-draft-adapter.tsclient/src/adapter/types.tsclient/src/adapter/wasm-adapter.tsclient/src/adapter/ws-adapter.tsclient/src/components/board/__tests__/PermanentCard.test.tsxclient/src/components/mana/ManaPaymentUI.tsxclient/src/components/mana/__tests__/ManaPaymentUI.test.tsxclient/src/components/modal/__tests__/ModeChoiceModal.test.tsxclient/src/components/modal/__tests__/TriggerOrderModal.test.tsxclient/src/components/multiplayer/ConcedeDialog.tsxclient/src/components/multiplayer/__tests__/ConcedeDialog.test.tsxclient/src/components/settings/PreferencesModal.tsxclient/src/game/__tests__/castPaymentMode.test.tsclient/src/game/__tests__/dispatchSplitEpochSoftlock.test.tsclient/src/game/__tests__/dispatchTurnControlPayCostQueue.test.tsclient/src/game/castPaymentMode.tsclient/src/game/controllers/__tests__/aiController.test.tsclient/src/game/controllers/aiController.tsclient/src/game/dispatch.tsclient/src/game/waitingForRegistry.tsclient/src/hooks/__tests__/useConcedeHandler.test.tsxclient/src/hooks/__tests__/useKeyboardShortcuts.test.tsxclient/src/hooks/useConcedeHandler.tsclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/de/multiplayer.jsonclient/src/i18n/locales/de/settings.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/en/multiplayer.jsonclient/src/i18n/locales/en/settings.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/es/multiplayer.jsonclient/src/i18n/locales/es/settings.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/fr/multiplayer.jsonclient/src/i18n/locales/fr/settings.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/it/multiplayer.jsonclient/src/i18n/locales/it/settings.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pl/multiplayer.jsonclient/src/i18n/locales/pl/settings.jsonclient/src/i18n/locales/pt/game.jsonclient/src/i18n/locales/pt/multiplayer.jsonclient/src/i18n/locales/pt/settings.jsonclient/src/network/__tests__/draftProtocol.test.tsclient/src/network/__tests__/protocol.test.tsclient/src/network/draftProtocol.tsclient/src/network/protocol.tsclient/src/pages/DraftPodPage.tsxclient/src/pages/GamePage.tsxclient/src/pages/__tests__/DraftPodPage.betweenGames.test.tsxclient/src/pages/__tests__/GamePage.bracketViolation.test.tsxclient/src/pages/__tests__/greenwardenDoubledTrigger.test.tsclient/src/pages/__tests__/optionalEffectChoiceTransition.test.tsxclient/src/providers/GameProvider.tsxclient/src/providers/__tests__/GameProvider.nativeEngine.test.tsxclient/src/services/__tests__/draftPersistence.test.tsclient/src/services/__tests__/fullTerminalResult.test.tsclient/src/services/__tests__/gamePersistence.test.tsclient/src/services/__tests__/intergameCommandLedger.test.tsclient/src/services/__tests__/multiplayerSession.test.tsclient/src/services/__tests__/p2pSession.test.tsclient/src/services/__tests__/p2pTerminalResult.test.tsclient/src/services/__tests__/scryfall.test.tsclient/src/services/draftPersistence.tsclient/src/services/fullTerminalResult.tsclient/src/services/gamePersistence.tsclient/src/services/intergameCommandLedger.tsclient/src/services/multiplayerSession.tsclient/src/services/p2pSession.tsclient/src/services/p2pTerminalResult.tsclient/src/stores/__tests__/multiplayerDraftStore.test.tsclient/src/stores/__tests__/multiplayerStore.test.tsclient/src/stores/multiplayerDraftStore.tsclient/src/stores/multiplayerStore.tsclient/src/stores/preferencesStore.tsclient/src/test/factories/engineAdapterFactory.tsclient/src/viewmodel/__tests__/cardActionChoice.test.tscrates/engine-wasm/src/lib.rscrates/engine/data/mtgjson-vintagecrates/engine/src/ai_support/candidates.rscrates/engine/src/ai_support/context.rscrates/engine/src/ai_support/filter.rscrates/engine/src/ai_support/mod.rscrates/engine/src/ai_support/payment_continuation.rscrates/engine/src/ai_support/targeted_exchange.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/costs.rscrates/engine/src/game/coverage.rscrates/engine/src/game/derived.rscrates/engine/src/game/effects/cast_from_zone.rscrates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/deal_damage.rscrates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/effects/exile_from_top_until.rscrates/engine/src/game/effects/free_cast_from_zones.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/pay.rscrates/engine/src/game/effects/prepare.rscrates/engine/src/game/effects/put_on_top.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_modes.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/engine_resolve_batch.rscrates/engine/src/game/interaction.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/mana_sources.rscrates/engine/src/game/marksman_tests.rscrates/engine/src/game/match_flow.rscrates/engine/src/game/replay.rscrates/engine/src/game/scenario.rscrates/engine/src/game/visibility.rscrates/engine/src/game/zone_pipeline.rscrates/engine/src/game/zones.rscrates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_class.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_effect/subject.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_effect/token.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_ir/context.rscrates/engine/src/parser/oracle_ir/doc.rscrates/engine/src/parser/oracle_ir/effect_chain.rscrates/engine/src/parser/oracle_ir/feature.rscrates/engine/src/parser/oracle_ir/relation.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rscrates/engine/src/parser/oracle_ir/trigger.rscrates/engine/src/parser/oracle_modal.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_separate_piles.rscrates/engine/src/parser/oracle_special.rscrates/engine/src/parser/oracle_static/keyword_grant.rscrates/engine/src/parser/oracle_tests.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/parser/oracle_util.rscrates/engine/src/types/ability.rscrates/engine/src/types/action_stable_order.rscrates/engine/src/types/actions.rscrates/engine/src/types/game_state.rscrates/engine/src/types/interaction.rscrates/engine/src/types/mana.rscrates/engine/src/types/match_config.rscrates/engine/src/types/mod.rscrates/engine/src/types/resolution.rscrates/engine/tests/integration/cr733_resolved_frame_transition.rscrates/engine/tests/integration/diluvian_primordial_6754.rscrates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rscrates/engine/tests/integration/invoke_calamity_free_cast.rscrates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rscrates/engine/tests/integration/issue_6477_wandering_archaic_optional_payment.rscrates/engine/tests/integration/issue_6677_wakandan_royal_guard.rscrates/engine/tests/integration/jagged_lightning_each_of_two_targets.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/riptide_gearhulk_5994.rscrates/engine/tests/integration/sacrificial_mana_choice.rscrates/engine/tests/integration/self_destruct_target_power.rscrates/engine/tests/integration/undying_malice_edict_sacrifice_5942.rscrates/manabrew-compat/src/lib.rscrates/mtgish-import/src/convert/mod.rscrates/phase-ai/Cargo.tomlcrates/phase-ai/baselines/perf-baseline.jsoncrates/phase-ai/src/auto_play.rscrates/phase-ai/src/bin/ai_bench_state.rscrates/phase-ai/src/bin/ai_commander.rscrates/phase-ai/src/bin/ai_duel.rscrates/phase-ai/src/bin/ai_gate.rscrates/phase-ai/src/bin/ai_perf_gate.rscrates/phase-ai/src/bin/ai_tune.rscrates/phase-ai/src/bin/attack_scaling_bench.rscrates/phase-ai/src/bin/combat_priority_bench.rscrates/phase-ai/src/bin/declare_attackers_bench.rscrates/phase-ai/src/bin/legal_actions_bench.rscrates/phase-ai/src/bin/pass_priority_bench.rscrates/phase-ai/src/bin/resolve_bench.rscrates/phase-ai/src/decision_kind.rscrates/phase-ai/src/duel_suite/perf.rscrates/phase-ai/src/mana_colors.rscrates/phase-ai/src/policies/discard_payoff.rscrates/phase-ai/src/policies/draw_payoff.rscrates/phase-ai/src/policies/self_cost.rscrates/phase-ai/src/policies/self_cost_value.rscrates/phase-ai/src/search.rscrates/phase-ai/src/tactical_gate.rscrates/phase-ai/tests/ai_commander_batch_equivalence.rscrates/phase-server/src/main.rscrates/phase-server/src/persistence.rscrates/server-core/src/client_message_wire_guard.rscrates/server-core/src/game_action_payload_guard.rscrates/server-core/src/lib.rscrates/server-core/src/p2p_backup_guard.rscrates/server-core/src/protocol.rscrates/server-core/src/reconnect.rscrates/server-core/src/session.rscrates/server-core/tests/game_action_payload_guard.rscrates/server-core/tests/lobby_wire_contract.rsdocs/parser-misparse-backlog.mdscripts/changelog/state.jsonscripts/gen-scryfall-sets.shscripts/lib/scryfall-fetch.shscripts/prelowered-ratchet.txt
💤 Files with no reviewable changes (6)
- client/src/adapter/tests/wasm-adapter.test.ts
- client/src/adapter/replay-adapter.ts
- client/src/pages/tests/greenwardenDoubledTrigger.test.ts
- client/src/pages/tests/optionalEffectChoiceTransition.test.tsx
- client/src/adapter/server-draft-adapter.ts
- crates/mtgish-import/src/convert/mod.rs
… chosen target (phase-rs#6559 review) The phase-rs#6507 predicate that binds a source-counter-gated rider pronoun to SelfRef was one degree too broad: it also fired on Revelation of Power ("Target creature gets +2/+2 until end of turn. If it has a counter on it, it also gains flying and lifelink"), whose intervening-if mis-scopes the bare "it" to CountersOn{Source}. Binding that grant to the source dropped flying/lifelink onto the one-shot Instant — the card lost its second sentence (engine_regress), and CR 608.2k does not reach it (the source is named by neither a cost nor a trigger condition). Narrow the binding: only rebind the pronoun to the source when NO earlier clause in the chain chose a typed target. Compute one gate at the chunk-subject site and reuse it at both consumers (the chunk-subject binding and the replace_target_with_parent guard): let binds_source_counter_pronoun = condition .is_some_and(condition_refs_source_object) && !chain_has_prior_typed_referent(builder.clauses(), false); chain_has_prior_typed_referent is true for Revelation of Power (its prior "Target creature gets +2/+2" is a Pump over a typed target) and false for every depletion-land / counter rider (whose prior clause is "Add mana" or "put a counter on ~", never a chosen target), so all 21 intended heals keep SelfRef while Revelation of Power's grant returns to ParentTarget. Chosen deliberately over chain_prior_referent_is_chosen_target, whose has_typed_target_widened early-out returns false for a pump-of-a-target. Adds source_counter_gate_over_prior_target_keeps_parent_not_self_ref (pins Revelation of Power's grant to ParentTarget) and extends the predicate unit test to drive the Sum/Difference two-operand walker arms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ae167b1 to
cf2e109
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/engine/src/parser/oracle_tests.rs (3)
23059-23062: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
has_unimplhelper.
fn has_unimplis defined identically three times in this file: at Line 23059, Line 23133, and Line 23222. Move it to a single module-level helper function and call it from each test.♻️ Proposed refactor
+fn ability_has_unimpl(def: &AbilityDefinition) -> bool { + matches!(def.effect.as_ref(), Effect::Unimplemented { .. }) + || def.sub_ability.as_deref().is_some_and(ability_has_unimpl) +} + #[test] fn depletion_land_rider_sacrifice_binds_self_ref() { ... - fn has_unimpl(def: &AbilityDefinition) -> bool { - matches!(def.effect.as_ref(), Effect::Unimplemented { .. }) - || def.sub_ability.as_deref().is_some_and(has_unimpl) - } assert!( - !parsed.abilities.iter().any(has_unimpl), + !parsed.abilities.iter().any(ability_has_unimpl), "no Unimplemented anywhere in the parse: {:#?}", parsed.abilities );Also applies to: 23133-23136, 23222-23225
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_tests.rs` around lines 23059 - 23062, Extract the duplicated has_unimpl helper into one module-level function in oracle_tests.rs, then remove the three local definitions and reuse the shared helper from each affected test. Preserve its existing recursive check for Effect::Unimplemented and nested sub_ability values.
23278-23315: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd reach-guards to this test, matching its siblings.
This test does not assert
parsed.parse_warnings.is_empty()or check forEffect::Unimplementedbefore asserting theHasCountersshape, unlikedepletion_land_rider_sacrifice_binds_self_ref(Line 23054-23067),typed_trigger_source_counter_rider_binds_self_ref_not_triggering_source(Line 23128-23145), andsource_counter_gate_over_prior_target_keeps_parent_not_self_ref(Line 23217-23230). Without these guards, a degraded parse that still happens to produce aGenericEffect/HasCountersshape through a different, unintended path would pass this test for the wrong reason. This test protects the same anaphora-binding logic that a prior review already flagged as a regression source (Revelation of Power), so the same reach-guard rigor applies here.✅ Proposed fix
let rider = parsed.abilities[0] .sub_ability .as_deref() .expect("conditional flying rider"); + assert!( + parsed.parse_warnings.is_empty(), + "expected zero parse warnings, got {:#?}", + parsed.parse_warnings + ); let Effect::GenericEffect { static_abilities, .. } = rider.effect.as_ref()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_tests.rs` around lines 23278 - 23315, Strengthen explicit_source_counter_gate_over_prior_target_stays_source_scoped with the same reach guards as its sibling tests: assert parsed.parse_warnings is empty and verify the relevant parsed effect is not Effect::Unimplemented before inspecting its GenericEffect/HasCounters structure. Keep the existing semantic assertions unchanged.
23278-23281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the CR 608.2c citation.
The comment cites "CR 122.1 + CR 608.2c" to justify that a prior typed target does not rewrite an explicit source subject. CR 608.2c reads: "The controller of the spell or ability follows its instructions in the order written." That rule governs order-of-resolution, not anaphor/subject binding. Cite a rule that actually supports subject-scope preservation (for example, CR 608.2k already used correctly elsewhere in this file for anaphora to a cost/trigger-named object) instead of CR 608.2c here.
Based on learnings, cite "CR 608.2c only when the comment is documenting the resolution of written instructions “in order” (not for general keyword-list behavior)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_tests.rs` around lines 23278 - 23281, Update the test comment above the affected test to replace the incorrect CR 608.2c citation with the applicable subject/anaphora-scope rule, such as CR 608.2k, while retaining CR 122.1 if relevant. Use CR 608.2c only for comments describing resolution of written instructions in order, not subject binding or keyword-list behavior.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 29364-29370: Remove the “CR 608.2k” citation from the issue
comment in the test documentation, leaving the Issue `#6507` and CR 122.1
references and the existing explanation of source-scoped versus
target/recipient-scoped counter checks unchanged.
- Around line 29392-29406: Extend the tests around condition_refs_source_object
with a positive QuantityCheck case that places CountersOn { scope:
ObjectScope::Source } in rhs and uses a fixed lhs. Keep the comparator and
expected true result consistent with existing positive cases, ensuring traversal
of QuantityCheck::rhs is covered.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_tests.rs`:
- Around line 23059-23062: Extract the duplicated has_unimpl helper into one
module-level function in oracle_tests.rs, then remove the three local
definitions and reuse the shared helper from each affected test. Preserve its
existing recursive check for Effect::Unimplemented and nested sub_ability
values.
- Around line 23278-23315: Strengthen
explicit_source_counter_gate_over_prior_target_stays_source_scoped with the same
reach guards as its sibling tests: assert parsed.parse_warnings is empty and
verify the relevant parsed effect is not Effect::Unimplemented before inspecting
its GenericEffect/HasCounters structure. Keep the existing semantic assertions
unchanged.
- Around line 23278-23281: Update the test comment above the affected test to
replace the incorrect CR 608.2c citation with the applicable
subject/anaphora-scope rule, such as CR 608.2k, while retaining CR 122.1 if
relevant. Use CR 608.2c only for comments describing resolution of written
instructions in order, not subject binding or keyword-list behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08c6b562-6bd9-40b7-bce5-acf0de7c159a
📒 Files selected for processing (6)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_tests.rscrates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs
- crates/engine/src/parser/oracle_nom/condition.rs
… chosen target (phase-rs#6559 review) The phase-rs#6507 predicate that binds a source-counter-gated rider pronoun to SelfRef was one degree too broad: it also fired on Revelation of Power ("Target creature gets +2/+2 until end of turn. If it has a counter on it, it also gains flying and lifelink"), whose intervening-if mis-scopes the bare "it" to CountersOn{Source}. Binding that grant to the source dropped flying/lifelink onto the one-shot Instant — the card lost its second sentence (engine_regress), and CR 608.2k does not reach it (the source is named by neither a cost nor a trigger condition). Narrow the binding: only rebind the pronoun to the source when NO earlier clause in the chain chose a typed target. Compute one gate at the chunk-subject site and reuse it at both consumers (the chunk-subject binding and the replace_target_with_parent guard): let binds_source_counter_pronoun = condition .is_some_and(condition_refs_source_object) && !chain_has_prior_typed_referent(builder.clauses(), false); chain_has_prior_typed_referent is true for Revelation of Power (its prior "Target creature gets +2/+2" is a Pump over a typed target) and false for every depletion-land / counter rider (whose prior clause is "Add mana" or "put a counter on ~", never a chosen target), so all 21 intended heals keep SelfRef while Revelation of Power's grant returns to ParentTarget. Chosen deliberately over chain_prior_referent_is_chosen_target, whose has_typed_target_widened early-out returns false for a pump-of-a-target. Adds source_counter_gate_over_prior_target_keeps_parent_not_self_ref (pins Revelation of Power's grant to ParentTarget) and extends the predicate unit test to drive the Sum/Difference two-operand walker arms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7983ff2 to
4e4571f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/parser/oracle_static/shared.rs`:
- Around line 3262-3268: Add a verified CR annotation beside the
QuantityRef::CountersOn source-to-recipient rebinding in the parser, citing CR
608.2k and CR 611.3a. Explain that conditions on an attached object rebind
Source to Recipient while explicit source references remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fa79c0e-5d99-4682-8dba-c49a48833667
📒 Files selected for processing (5)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_static/mod.rscrates/engine/src/parser/oracle_static/shared.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/src/parser/oracle_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/src/parser/oracle_effect/mod.rs
- crates/engine/src/parser/oracle_tests.rs
|
Current-head maintainer hold for |
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer completion verified on e841edb: source-counter rebinding is constrained to the bare-pronoun/prior-target grammar, uses the shared recursive static-condition rebinder, and includes focused source/recipient plus RHS traversal coverage. Stale pre-rebase threads were resolved; current-head CI has been requested and merge-queue checks remain authoritative.
… chosen target (phase-rs#6559 review) The phase-rs#6507 predicate that binds a source-counter-gated rider pronoun to SelfRef was one degree too broad: it also fired on Revelation of Power ("Target creature gets +2/+2 until end of turn. If it has a counter on it, it also gains flying and lifelink"), whose intervening-if mis-scopes the bare "it" to CountersOn{Source}. Binding that grant to the source dropped flying/lifelink onto the one-shot Instant — the card lost its second sentence (engine_regress), and CR 608.2k does not reach it (the source is named by neither a cost nor a trigger condition). Narrow the binding: only rebind the pronoun to the source when NO earlier clause in the chain chose a typed target. Compute one gate at the chunk-subject site and reuse it at both consumers (the chunk-subject binding and the replace_target_with_parent guard): let binds_source_counter_pronoun = condition .is_some_and(condition_refs_source_object) && !chain_has_prior_typed_referent(builder.clauses(), false); chain_has_prior_typed_referent is true for Revelation of Power (its prior "Target creature gets +2/+2" is a Pump over a typed target) and false for every depletion-land / counter rider (whose prior clause is "Add mana" or "put a counter on ~", never a chosen target), so all 21 intended heals keep SelfRef while Revelation of Power's grant returns to ParentTarget. Chosen deliberately over chain_prior_referent_is_chosen_target, whose has_typed_target_widened early-out returns false for a pump-of-a-target. Adds source_counter_gate_over_prior_target_keeps_parent_not_self_ref (pins Revelation of Power's grant to ParentTarget) and extends the predicate unit test to drive the Sum/Difference two-operand walker arms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ffc15ba to
90b2aea
Compare
|
Current-head maintainer hold for The rebase started a fresh CI run. Rust lint/parser gate and WASM are in progress; Rust-test shards, card-data/parse-diff, Tauri, frontend, and Lobby checks are pending/queued. The visible 17-card parse artifact predates this head, so it is not current-head evidence yet. Auto-merge is disabled until current artifacts complete successfully. No merge-queue action during this hold. |
matthewevans
left a comment
There was a problem hiding this comment.
Re-approving the unchanged, reviewed maintainer completion after rebase onto the current merge-queue base.
|
Correction: the current head is Current CI has restarted and is not yet merge evidence; auto-merge is disabled. No merge-queue action until current-head required checks and the current parse artifact complete successfully. |
…Gemstone Mine phase-rs#6507) The depletion-land sacrifice rider ("If there are no mining counters on this land, sacrifice it.") parsed to Sacrifice{ParentTarget}. A mana ability has no targets (CR 605.1a), so ParentTarget resolved to an empty set and the sacrifice silently no-op'd — the land never left play. Root cause is a parse-time anaphor mis-binding, not a runtime gap: the chunk-subject threading in the effect-chain parser already binds a bare "it" to SelfRef when the gating condition references the source object, via condition_refs_source_object. That predicate recognized the source-tapped / source-entered / source-attached conditions but not a source-scoped counter threshold (QuantityCheck over CountersOn{Source}), so the counter-gated riders fell through to ParentTarget (and, on typed triggers, to TriggeringSource). Extend condition_refs_source_object with a QuantityCheck arm that returns true when either side reads counters on the source, via a new exhaustive QuantityExpr walker (no wildcard — a future variant must be classified). This single predicate extension drives both existing consumers: the chunk-subject threading now supplies SelfRef, and the ParentTarget rewrite guard now skips these chunks. Bindings become source-correct for the Mercadian depletion lands (Peat Bog, Hickory Woodlot, Remote Farm, Sandstone Needle, Saprazzan Skerry), Gemstone Mine, Tourach's Gate, Daredevil Dragster, Last Light of Durin's Day, ED-E, and the whole source-counter-conditioned rider class (~21 cards). No runtime files change. CR 122.1 + CR 608.2k annotate the new arm. Closes phase-rs#6507 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… chosen target (phase-rs#6559 review) The phase-rs#6507 predicate that binds a source-counter-gated rider pronoun to SelfRef was one degree too broad: it also fired on Revelation of Power ("Target creature gets +2/+2 until end of turn. If it has a counter on it, it also gains flying and lifelink"), whose intervening-if mis-scopes the bare "it" to CountersOn{Source}. Binding that grant to the source dropped flying/lifelink onto the one-shot Instant — the card lost its second sentence (engine_regress), and CR 608.2k does not reach it (the source is named by neither a cost nor a trigger condition). Narrow the binding: only rebind the pronoun to the source when NO earlier clause in the chain chose a typed target. Compute one gate at the chunk-subject site and reuse it at both consumers (the chunk-subject binding and the replace_target_with_parent guard): let binds_source_counter_pronoun = condition .is_some_and(condition_refs_source_object) && !chain_has_prior_typed_referent(builder.clauses(), false); chain_has_prior_typed_referent is true for Revelation of Power (its prior "Target creature gets +2/+2" is a Pump over a typed target) and false for every depletion-land / counter rider (whose prior clause is "Add mana" or "put a counter on ~", never a chosen target), so all 21 intended heals keep SelfRef while Revelation of Power's grant returns to ParentTarget. Chosen deliberately over chain_prior_referent_is_chosen_target, whose has_typed_target_widened early-out returns false for a pump-of-a-target. Adds source_counter_gate_over_prior_target_keeps_parent_not_self_ref (pins Revelation of Power's grant to ParentTarget) and extends the predicate unit test to drive the Sum/Difference two-operand walker arms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
90b2aea to
abcbf9d
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Re-approving the unchanged maintainer completion after the final rebase onto current main.
|
Current-head hold for Current CI is still running (Rust lint/parser gate, both Rust shards, card data/coverage, and frontend); WASM, Tauri, Lobby, security, contributor trust, and CodeRabbit are successful. The 17-card/5-signature parse artifact predates this head, so it is not current-head evidence yet. Auto-merge is disabled. Do not queue until current CI and parse artifacts complete successfully. |
Summary
Fixes Gemstone Mine (#6507) and the whole source-counter-conditioned rider class: the depletion-land sacrifice rider — "{T}, Remove a mining counter from this land: Add one mana of any color. If there are no mining counters on this land, sacrifice it." — never sacrificed the land after its last counter was removed.
Root cause
Parse-time anaphor mis-binding, not a runtime gap. The rider's sub-ability parsed to
Sacrifice { target: ParentTarget }. A mana ability has no targets (CR 605.1a), soParentTargetresolves to an empty set at resolution (game/effects/sacrifice.rs—effect_object_targets(ParentTarget, [])is empty → the sacrifice silently no-ops and the land survives).The effect-chain parser already binds a bare "it" to
SelfRefwhen the gating condition references the source object, viacondition_refs_source_object. That predicate recognized source-tapped / source-entered / source-attached conditions, but not a source-scoped counter threshold (QuantityCheckoverCountersOn { scope: Source }) — the exact shape these riders produce. So the counter-gated body pronoun fell through toParentTarget(and, on typed triggers, toTriggeringSource).Fix
One additive predicate extension in
crates/engine/src/parser/oracle_effect/mod.rs(+41/-0):QuantityExprwalkerquantity_expr_reads_source_counters(no wildcard arm — a future variant must be classified; mirrorsquantity_expr_uses_recipient);QuantityCheck { lhs, rhs, .. }arm incondition_refs_source_objectreturning true when either side reads counters on the source.This single change drives both existing consumers of the predicate: the chunk-subject threading (mod.rs:28564) now supplies
SelfRef, and theParentTargetrewrite guard (mod.rs:29335) now skips these chunks. No runtime files change; the AST is now what the card says (SelfRef), andsacrifice.rs's existingSelfRefpool resolution + CR 400.7 epoch guard do the rest.Corrects the binding for ~21 cards: the 5 Mercadian depletion lands, Gemstone Mine, Tourach's Gate, Contested Game Ball, Daredevil Dragster, Dawn of a New Age, Evolved Spinoderm (
Sacrifice ParentTarget → SelfRef); Blood Spatter Analysis, Charitable Levy, Decree of Silence, Last Light of Durin's Day, The Heron Moon, ED-E (Sacrifice/PutCounter TriggeringSource → SelfRef); Heirloom Mirror, Ludevic's Test Subject, Replicating Ring, Smoldering Egg (RemoveCounter ParentTarget → SelfRef). Grasping Shadows / Soulcipher Board flipTransform SelfRef → ParentTarget, which is behavior-neutral (the transform effect handler falls back to the source on empty targets).Files changed
crates/engine/src/parser/oracle_effect/mod.rs— the fix (helper + match arm)crates/engine/src/parser/oracle_effect/tests.rs— predicate unit test (Source→true incl. wrapped/Not/And; Target/Recipient scopes→false)crates/engine/src/parser/oracle_tests.rs— 2 parser SHAPE tests (Gemstone Mine, Last Light) with reach-guardscrates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs— 5 runtime testscrates/engine/tests/integration/main.rs— mod lineCR references
CR 122.1(counters) +CR 608.2k(an effect referring to an untargeted object previously referred to by the ability still affects it) — the new predicate arm / helper.CR 605.1a(mana ability requires no target) +CR 605.3b(mana ability resolves immediately) +CR 701.21a(sacrifice) — test annotations.Implementation method (required)
Method: /engine-implementer
Track
Developer
LLM
Model: claude-opus-4-8[1m]
Thinking: high
Verification
Required checks ran clean.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo test -p engine --lib— 17593 passed, 0 failed, 6 ignored (baseline 17590 + 3 new)cargo test -p engine --test integration— 3874 passed, 0 failed, 2 ignored (baseline 3869 + 5 new)cargo fmt --all -- --check— clean./scripts/gen-card-data.sh— regenerated; parse-diff audit of the source-counter rider shape class: 23 cards changed, all either correctness heals or behavior-neutral (Transform empty-target→source fallback), zero regressions.RED/GREEN: tests 1/3/4/5 + both shape tests fail on the pre-fix
ParentTarget/TriggeringSourcebinding and pass after; test 2 is the paired over-trigger negative with positive reach-guards.Gate A
Gate A PASS head=1c74fedb5bef8d76fb45de2c5e22833208179d1d base=6ae8737cdab0fa1ed291cad0f0808473a90f4cf8
Anchored on
crates/engine/src/parser/oracle_trigger.rs:1297— trigger-body parse constructseffect_ctxwithsubject: Some(trigger_subject.clone())(the established subject-threading seam that already binds trigger-borne riders correctly).crates/engine/src/parser/oracle_effect/mod.rs:2876— delayed-trigger body parse threadsinner_ctx.subject = Some(TargetFilter::SelfRef)for the self-referential case — the exactSelfRefsubject-threading this change completes for the counter-gated chunk path (mod.rs:28564).Final review-impl
Final review-impl PASS head=1c74fedb5bef8d76fb45de2c5e22833208179d1d
Claimed parse impact
Validation Failures
None.
CI Failures
None.
Tier: Frontier
Summary by CodeRabbit
Bug Fixes
Tests